Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

08MQ reproduction #121

Merged
merged 129 commits into from
Jan 4, 2024
Merged

08MQ reproduction #121

merged 129 commits into from
Jan 4, 2024

Conversation

bclenet
Copy link
Collaborator

@bclenet bclenet commented Oct 9, 2023

This Pull Request is related to issue #120

Changes proposed in this Pull Request:

  • Pipeline reproduction

Checklist:

  • Descriptive title
  • Targets the main branch
  • Changes are functional
  • My code is explicit and comments were added to it
  • Code conforms with PEP8
  • Tests were added for the changes and they complete successfully
  • Existing tests were updated (if needed) and they complete successfully
  • Documentation was updated

@cmaumet
Copy link
Contributor

cmaumet commented Nov 30, 2023

@cmaumet

Voici un lien vers le fichier team_08MQ.py dans le dernier commit de la PR

Dans ce fichier, les numéros de lignes associés à la question sur la définition du modèle : 406 409 412

Dans ce fichier, les numéros de lignes associés à la question sur le masque MNI : 608 779

Dans ce fichier, le numéro de ligne associé à la question sur randomise : 782

Thank you!

Copy link
Contributor

@cmaumet cmaumet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bclenet: I've reviewd the pipeline code as suggested and did not spot any issues. One thing that would help me doing a second check would be to get the corresponding fsf file and open it into FSL. Is this feasible with nipype? (I think the equivalent is possible for SPM but I am unsure this is available for FSL).

I have also answered in this review your 3 questions, when can continue the discussion directly in the comments.

Let me know if there is any other check I can help with!

narps_open/pipelines/team_08MQ.py Outdated Show resolved Hide resolved
# FLAMEO Node - Estimate model
estimate_model = Node(FLAMEO(), name = 'estimate_model')
estimate_model.inputs.run_mode = 'fe' # Fixed effect
estimate_model.inputs.mask_file = Info.standard_image('MNI152_T1_2mm_brain_mask.nii.gz')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a mask created for each subject (as each subject might have different voxels missing in the MNI space due to signal dropouts). You can use the intersection of the masks generated by the run-level GLM. Does this help?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks, let's do this !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed on Dec 1. It looks like feat is creating the mask in the background we can check in the paper that reproduce feat in nipype how they dealt with this.

narps_open/pipelines/team_08MQ.py Show resolved Hide resolved
randomise.inputs.vox_p_values = True
randomise.inputs.c_thresh = 0.05
randomise.inputs.tfce_E = 0.01
randomise.inputs.mask = Info.standard_image('MNI152_T1_2mm_brain_mask.nii.gz')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it mandatory to specify a mask? If not we can leave this empty so that randomise comupte the mask itself. If we have to provide a mask, for the same reason as above (i.e. some voxels might be missing because of signal-dropouts), we need to use here a study-specific mask. I think that typically we use the intersection of all the subject-specific masks (but maybe this is something to check -- I can look into it, let me know!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not mandatory to provide a mask, I can remove the line then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erratum : the FLAMEO node (couple of lines earlier) in the group level analysis uses a mandatory mask input : I guess we will have to compute the intersection of all previously computed subject-specific masks then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the same mask for randomize as well.

narps_open/pipelines/team_08MQ.py Outdated Show resolved Hide resolved
@bclenet
Copy link
Collaborator Author

bclenet commented Nov 30, 2023

I can get fsf files for the run level analysis (as soon as it runs...) : it is the output of the model_design (Level1Design) node. Would that help ?

@bclenet bclenet merged commit 27b567e into Inria-Empenn:main Jan 4, 2024
@bclenet bclenet deleted the 08MQ_reproduction branch January 31, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants